-
Notifications
You must be signed in to change notification settings - Fork 31
Fixed dictionary item value serialization issue - Part 2 #445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed dictionary item value serialization issue - Part 2 #445
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we set the new property in the API method the issue was opened for? Otherwise the issue won't be fixed.
arangodb-net-standard.Test/Serialization/JsonNetApiClientSerializationTest.cs
Show resolved
Hide resolved
You mean in |
…kProperties-not-serializing-correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
- Do you think we could/should simplify the name slightly from
applySerializationOptionsToObjectValuesInDictionaries
toapplySerializationOptionsToDictionaryValues
? Is the meaning still explicit enough after renaming?
var content = GetContent(body, new ApiClientSerializationOptions(true, true)); | ||
using (var response = await _transport.PostAsync(uri, content, token: token).ConfigureAwait(false)) | ||
var content = GetContent(body, | ||
serializationOptions ?? new ApiClientSerializationOptions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think serializationOptions
must be exposed to callers of this API method. ArangoDB Web API accepts a JSON with camelCase, we don't want callers to change the standard JSON that the library/driver sends to ArangoDB.
Instead we should turn on the option, like so:
var content = GetContent(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
applySerializationOptionsToObjectValuesInDictionaries: true));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that you asked about this in your previous comment: #445 (comment)
I missed that.
From what I understand, the option should be hardcoded by us in the API method because we don't want to allow other options to be modified.
One gotchat is that null properties provided in LinkProperties will be ignored, because our serialization options for PostCreateViewAsync
ignore null values. Is that acceptable? I don't understand enough about views and fields to know if it is gonna be a problem.
I see the following mentioned on LinksProperties.IncludeAllFields:
/// If set to true, then process all document attributes.
/// Otherwise, only consider attributes mentioned in fields.
/// Attributes not explicitly specified in fields will be
/// processed with default link properties, i.e. null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think
serializationOptions
must be exposed to callers of this API method. ArangoDB Web API accepts a JSON with camelCase, we don't want callers to change the standard JSON that the library/driver sends to ArangoDB.Instead we should turn on the option, like so:
var content = GetContent(body, new ApiClientSerializationOptions( useCamelCasePropertyNames: true, ignoreNullValues: true, applySerializationOptionsToObjectValuesInDictionaries: true));
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that you asked about this in your previous comment: #445 (comment)
I missed that.
From what I understand, the option should be hardcoded by us in the API method because we don't want to allow other options to be modified. One gotchat is that null properties provided in LinkProperties will be ignored, because our serialization options for
PostCreateViewAsync
ignore null values. Is that acceptable? I don't understand enough about views and fields to know if it is gonna be a problem.I see the following mentioned on LinksProperties.IncludeAllFields:
/// If set to true, then process all document attributes. /// Otherwise, only consider attributes mentioned in fields. /// Attributes not explicitly specified in fields will be /// processed with default link properties, i.e. null.
If IncludeAllFields
is set to true
, then all document attributes are processed. The ones not specified in Fields
are processed with a default value of null
.
- When
IncludeAllFields == true
andignoreNullValues == true or false
, it's not a problem.null
fields specified by the caller and removed by the serialization are replaced and processed asnull
by the server by default. - When
IncludeAllFields == false
andignoreNullValues == true
, it could cause a problem fornull
attributes specified by the caller, because the caller clearly intends to process the attributes asnull
, but the serialization will remove/ignore it. - When
IncludeAllFields == false
andignoreNullValues == false
, it will not cause any problem becausenull
attributes specified by the caller will be processed asnull
as intended by the caller.
So, I think it makes sense to setignoreNullValues
tofalse
. @DiscoPYF , your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting ignoreNullValues
to false
, causes a lot of errors with our existing tests because several property values are not specified. I think this will break a lot of existing code that are not specifying the whole view body. For better backward compatibility, I've added a new parameter ignoreNullValuesOnSerialization
which defaults to true
. @DiscoPYF , your thoughts please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a closer look in the coming days. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance, some thoughts:
What you did makes sense to me (adding a new method parameter ignoreNullValuesOnSerialization
to PostCreateViewAsync
and others), letting the caller decide based on what they are providing.
I'm not able to review fully right now (limited time) but will do in coming days.
Alternatives:
We can allow specifying the dictionary options individually, letting us cover all cases. For example: add a new property in ApiClientSerializationOptions
:
public class ApiClientSerializationOptions
{
/*...*/
/// <summary>
/// Whether dictionary values should be serialized with the same options
/// as normal objects.
/// </summary>
/// <remarks>
/// If false, you can use <see cref="DictionaryOptions"/>
/// to control individual options for dictionary serialization.
/// </remarks>
public bool ApplySerializationOptionsToDictionaryValues { get; set; }
/// <summary>
/// Serialization options to apply specifically to dictionary values.
/// </summary>
/// <remarks>
/// If <see cref="ApplySerializationOptionsToDictionaryValues"/> is true,
/// this property is ignored.
/// If <see cref="ApplySerializationOptionsToDictionaryValues"/> is false and
/// <see cref="DictionaryOptions"/> is null, then dictionary values are serialized
/// as provided (untouched).
/// </remarks>
public ApiClientSerializationOptions DictionaryOptions { get; set; }
}
Perhaps create a new type for dictionary options:
public interface ISerializationOptions // For convenience to have a source of truth for available options.
{
public bool UseCamelCasePropertyNames { get; set; }
public bool IgnoreNullValues { get; set; }
public bool IgnoreMissingMember { get; set; }
public bool UseStringEnumConversion { get; set; }
}
public class ApiClientSerializationOptions : ISerializationOptions
{
/*...*/
public bool ApplySerializationOptionsToDictionaryValues { get; set; }
public DictionaryValuesSerializationOptions DictionaryOptions { get; set; }
}
public class DictionaryValuesSerializationOptions : ISerializationOptions
{
/*...*/
}
Maybe ApplySerializationOptionsToDictionaryValues
can be considered redundant then. It would be more of a convenience for any consumer that want to align dictionary serialization without having to specify all options again.
With this new property, we (as the library) can do the following inside PostCreateViewAsync
:
var content = GetContent(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
DictionaryOptions: new DictionaryValuesSerializationOptions()
{
useCamelCasePropertyNames: true,
ignoreNullValues: IncludeAllFields // not sure if it makes sense
}));
I don't know if this also covers advanced scenarios such as nesting of links, e.g. what happens when someone sends:
new ViewDetails()
{
Links = new Dictionary<string, LinksProperties>()
{
["Files"] = new LinksProperties()
{
Fields = Dictionary<string, LinksProperties>()
{
["Properties"] = new LinksProperties()
{
/*...*/
}
}
}
}
}
Not sure if this is even valid, I don't know enough about the view feature. Perhaps best to leave it to the caller like you did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another look and this approach could be used together with the parameter ignoreNullValuesOnSerialization
so that it is only applied to links and fields. We could rename this parameter and do something like:
public virtual async Task<ViewResponse> PostCreateViewAsync(
ViewDetails body,
bool ignoreNullLinksAndFields = false,
CancellationToken token = default)
{
var content = GetContent(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
DictionaryOptions = new DictionaryValuesSerializationOptions
{
useCamelCasePropertyNames: true,
ignoreNullValues: ignoreNullLinksAndFields
}));
}
Not blocker, just a thought.
Good idea to simplify the name. Done. |
@DiscoPYF , I've also applied these changes to |
…plied changes to PatchViewPropertiesAsync() and PutViewPropertiesAsync()
…c(), PatchViewPropertiesAsync() and PutViewPropertiesAsync()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjoubert I reviewed and looks good to me. Before approving:
It would be great to add tests for PostCreateViewAsync
that cover this particular case, so that we make sure it's working and have a way to check it in future (e.g. if we end up making more changes to the serializer).
Something like: PostCreateViewAsync_ShouldSerializeLinkPropertiesWithCamelCase
What do you think?
var content = GetContent(body, new ApiClientSerializationOptions(true, true)); | ||
using (var response = await _transport.PostAsync(uri, content, token: token).ConfigureAwait(false)) | ||
var content = GetContent(body, | ||
serializationOptions ?? new ApiClientSerializationOptions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another look and this approach could be used together with the parameter ignoreNullValuesOnSerialization
so that it is only applied to links and fields. We could rename this parameter and do something like:
public virtual async Task<ViewResponse> PostCreateViewAsync(
ViewDetails body,
bool ignoreNullLinksAndFields = false,
CancellationToken token = default)
{
var content = GetContent(body, new ApiClientSerializationOptions(
useCamelCasePropertyNames: true,
ignoreNullValues: true,
DictionaryOptions = new DictionaryValuesSerializationOptions
{
useCamelCasePropertyNames: true,
ignoreNullValues: ignoreNullLinksAndFields
}));
}
Not blocker, just a thought.
Solves Issue #400
This is a follow-on PR from #401
Issues raised in the comments in the old PR have been fixed by this one.